Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Client] Correct limit check in throttler thread #5186

Merged
merged 2 commits into from
Apr 10, 2023
Merged

[Client] Correct limit check in throttler thread #5186

merged 2 commits into from
Apr 10, 2023

Conversation

BrianNixon
Copy link
Contributor

@BrianNixon BrianNixon commented Apr 8, 2023 •
edited
Loading

Fixes #5072 more

Without this, if the CPU limit is 100%, the throttler thread suspends all tasks, resumes them all 1 s later, then goes to sleep forever (meaning subsequent changes to the limit are never applied)

(Commit 3b1ebfd changed the ‘nothing to do’ check to limit == 0 at the same time as implementing CLIENT_STATE::current_cpu_usage_limit() which makes that condition impossible)

@AenBleidd
Copy link
Member

@davidpanderson, could you please take a look at this PR?

@AenBleidd AenBleidd added this to the Client/Manager 7.22.1 milestone Apr 8, 2023
@davidpanderson
Copy link
Contributor

Good catch! This PR is fine and can be merged.
You can change the 99.99 to 100 since current_cpu_usage_limit() returns that if the pref is > 99.99

@BrianNixon
Copy link
Contributor Author

In general it’s good practice to avoid strict equality checks with doubles. While a literal 100 is not a general case, a threshold comparison keeps the code slightly more resistant to future inadvertent breakage. (Imagine somebody decides to replace 100 with limit_from_some_clever_dynamic_calculation which comes out close to, but not exactly, 100.)

@AenBleidd AenBleidd merged commit 1560a00 into BOINC:master Apr 10, 2023
@BrianNixon BrianNixon deleted the throttler_fix branch April 10, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AenBleidd AenBleidd AenBleidd approved these changes

@davidpanderson davidpanderson Awaiting requested review from davidpanderson

Assignees

@davidpanderson davidpanderson

Labels
C: Client P: Major PR: Bugfix
Projects
None yet
Milestone
Client/Manager 7.22.1
Development

Successfully merging this pull request may close these issues.

7.22.0 CPU usage FULL THROTTLE on macOS Ventura.2
3 participants
@BrianNixon @AenBleidd @davidpanderson